Closed
Conversation
Removes a few lines of C++ code while making `isIPv4()` about 3x faster. `isIPv6()` and `isIP()` for the IPv6 case stay about the same. I removed the homegrown `isIPv4()` in lib/dns.js that utilized a lookup table. It is in fact a little faster than the new `isIPv4()` function but: 1. the difference is only measurable at around 10M iterations, and 2. the function is a "probably IPv4" heuristic, not a proper validator
nodejs-github-bot
added
c++
hybrist
approved these changes
Jan 26, 2018
richardlau
approved these changes
Jan 26, 2018
jasnell
approved these changes
Jan 26, 2018
cjihrig
approved these changes
Jan 26, 2018
joyeecheung
approved these changes
Jan 27, 2018
addaleax
approved these changes
Jan 28, 2018
Member
addaleax
added
the
author ready
bnoordhuis
added a commit
to bnoordhuis/io.js
that referenced
this pull request
Jan 29, 2018
PR-URL: nodejs#18398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
bnoordhuis
added a commit
to bnoordhuis/io.js
that referenced
this pull request
Jan 29, 2018
Removes a few lines of C++ code while making `isIPv4()` about 3x faster. `isIPv6()` and `isIP()` for the IPv6 case stay about the same. I removed the homegrown `isIPv4()` in lib/dns.js that utilized a lookup table. It is in fact a little faster than the new `isIPv4()` function but: 1. The difference is only measurable at around 10M iterations, and 2. The function is a "probably IPv4" heuristic, not a proper validator. PR-URL: nodejs#18398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Member
Author
|
Landed in 98d1110...742ae61. Some infrastructural issues on the arm buildbots, everything else was green. |
evanlucas
pushed a commit
that referenced
this pull request
Jan 30, 2018
PR-URL: #18398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
ChALkeR
added
the
performance
addaleax
removed
the
author ready
Contributor
|
Should this be backported to |
MylesBorins
added
lts-watch-v6.x
baking-for-lts
Contributor
|
ping again re: backport to 9.x. this was only partially backported to an earlier release. |
MayaLekova
pushed a commit
to MayaLekova/node
that referenced
this pull request
May 8, 2018
PR-URL: nodejs#18398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MayaLekova
pushed a commit
to MayaLekova/node
that referenced
this pull request
May 8, 2018
Removes a few lines of C++ code while making `isIPv4()` about 3x faster. `isIPv6()` and `isIP()` for the IPv6 case stay about the same. I removed the homegrown `isIPv4()` in lib/dns.js that utilized a lookup table. It is in fact a little faster than the new `isIPv4()` function but: 1. The difference is only measurable at around 10M iterations, and 2. The function is a "probably IPv4" heuristic, not a proper validator. PR-URL: nodejs#18398 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
MylesBorins
removed
the
baking-for-lts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes a few lines of C++ code while making
isIPv4()about 3x faster.isIPv6()andisIP()for the IPv6 case stay about the same.I removed the homegrown
isIPv4()in lib/dns.js that utilized a lookuptable. It is in fact a little faster than the new
isIPv4()functionbut: